Skip to content

Conversation

sosnovsky
Copy link
Collaborator

This PR fixes build on macOS

close #5043


Tests (delete all except exactly one):

  • Does not need tests (refactor only, docs or internal changes)

To be filled by reviewers

I have reviewed that this PR... (tick whichever items you personally focused on during this review):

  • addresses the issue it closes (if any)
  • code is readable and understandable
  • is accompanied with tests, or tests are not needed
  • is free of vulnerabilities
  • is documented clearly and usefully, or doesn't need documentation

@sosnovsky sosnovsky requested a review from rrrooommmaaa March 28, 2023 06:59
@sosnovsky sosnovsky marked this pull request as ready for review March 28, 2023 06:59
scripts/build.sh Outdated
STREAMS_FILES=$OUTDIR/lib/streams/*
# patch isUint8Array until https://github.com/openpgpjs/web-stream-tools/pull/23 is resolved
ISUINT8ARRAY_REGEX="s/^(\s*)return\x20Uint8Array\.prototype\.isPrototypeOf\(input\);/\1return\x20Uint8Array\.prototype\.isPrototypeOf\(input\)\x20\|\|\x20globalThis\.Uint8Array\.prototype\.isPrototypeOf\(input\);/mg"
ISUINT8ARRAY_REGEX="s/^(\s*)return\x20Uint8Array\.prototype\.isPrototypeOf\(input\);/\1return\x20Uint8Array\.prototype\.isPrototypeOf\(input\)\x20\|\|\x20globalThis\.Uint8Array\.prototype\.isPrototypeOf\(input\);/g"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rrrooommmaaa build on macOS failed because of m flag. I removed it and seems everything works well even without it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The patch only matters for Firefox, and Firefox is not tested automatically, so we can't tell whether it works well.
Does this statement actually patched the file -- does the new file differ?
The function isUint8Array should look like this:

function isUint8Array(input) {
  return Uint8Array.prototype.isPrototypeOf(input) || globalThis.Uint8Array.prototype.isPrototypeOf(input);
}

The m flag is supposed to give ^ meaning "beginning of any line" instead of "beginning of file"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I re-checked - files were not patched without m flag.
But I updated ISUINT8ARRAY_REGEX, so it's not using ^, and now files are patched.
What do you think about updated regex, is it ok?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's ok to not use ^,
It'd be nicer to leave "one or more whitespace" before return with (\s+) and \1 in this case.
What is wrong with sed on MacOS? Is it some old version? Can't it be updated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated regex to use (\s*) (as it didn't work after changing to (\s+))

What is wrong with sed on MacOS? Is it some old version? Can't it be updated?

Yes, macOS includes an older version of sed which doesn't support some functionality.
It can be updated by installing another version and then linking it to the default sed command.
I thought about adding this info to our README, but as this ISUINT8ARRAY_REGEX patch is temporary (before openpgpjs/web-stream-tools#23 is fixed) I think updating regex should be ok for now.

@sosnovsky sosnovsky merged commit c79e882 into master Mar 29, 2023
@sosnovsky sosnovsky deleted the bugfix/issue-5043-mac-build branch March 29, 2023 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Build fails on macOS

2 participants